Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce run-time language switching #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

niklas
Copy link

@niklas niklas commented Oct 24, 2017

This incorporates all present translations into one build as described in #2 . Instead of a symbolic link to the compile-time language, a switch is generated for every translation module. This switch contains all the functions of the original(s), but they take a Language as an extra (preceeding) argument, delegating the translation itself to the corresponding module for that language.

Migration of apps is needed, instructions in README.md.

NOTE: This breaks the currently implemented behaviour of "one language per build".
NOTE2: This might still contain some old behavior. I will clean that up if you want that PR :-)

by introducing types describing their intent.
This incorporates all present translations into one build. Instead of a
symlink to the compile-time language, a switch is generated for every
translation module. This switch contains all the functions of the
original(s), but they take a Language as an extra (preceeding) argument,
delegating the translation itself to the corresponding module for that
language.

Migration of apps is needed, instructions in README.md.

NOTE: This breaks the currently implemented behaviour of "one language
per build".
@felixLam
Copy link
Member

Hi Niklas, thank you for your contribution! This looks very promising. We will try our best to get your code reviewed ASAP.

@felixLam
Copy link
Member

The test appear to be failing due to an additional newline introduced here.

Copy link
Member

@felixLam felixLam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some very general feedback on coding-style. More substantive feedback will follow.

elm-i18n-generator --language De,Pl,En,Fr --genswitch
```

#### Migrate legacy symlinked translations to switchables
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should maybe not just yet call it legacy. I believe that both options could reside side-by-side with advantages for each approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we need to implement this in a way that does not break existing implementation unless they use new cli flags.

@@ -7,7 +7,7 @@ module CSV.Export exposing (generate)
-}

import CSV.Template
import Localized
import Localized exposing (..)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert changes to this file. There do not appear to be substantive changes to this file. We avoid using exposing (..). We usually choose module names and exposed names to go well together: Localized.Element is much clearer than Element IMHO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might become quite "loud" because of the types I introduced. While it is easier to find the location of the definition, it forces a lot of repetition. I'll change it if you insist, but I prefer the slickness (TM).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We side with https://github.com/stil4m/elm-analyse on this. Keeping this style-guide also reduce the size of the diff in this PR.

@@ -15,7 +15,7 @@ elements.

import Csv
import Dict
import Localized
import Localized exposing (..)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate the changes from String to named type alias! Please keep the named import though: again Localized.Module is easier to read and understand than just Module



generateForModule : List (List String) -> List Localized.Element
-- Generate the source code for each module based on the lines
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using the latest version of elm-format?

@niklas
Copy link
Author

niklas commented Oct 25, 2017

I refactored quite a lot, hope it does not look too ruby-ish. The newline has been ele.. eli.. removed.

@tmbull
Copy link

tmbull commented May 23, 2018

@felixLam @niklas Is there any reason this PR can't get updated and merged? We are wanting to add localization to our elm app, and this library looks very nice and easy to use. However, run-time language switching is a requirement for us.

@woylie
Copy link

woylie commented Oct 4, 2018

Any news on this?

@klaftertief
Copy link
Collaborator

I'm not co-maintaining this library anymore so can't give any updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants